Added a synchronous write loop for connections. #1392
Conversation
lukebakken
force-pushed
the
rabbitmq-dotnet-client-1389
branch
from
8a40935
to
9132e7f
Compare
lukebakken
force-pushed
the
rabbitmq-dotnet-client-1389
branch
2 times, most recently
from
3a6cbf1
to
a905764
Compare
lukebakken
force-pushed
the
rabbitmq-dotnet-client-1389
branch
from
a905764
to
1bbe3b8
Compare
|
Is this worthwhile pulling in given the direction the client is heading to? To me that looks more like a bandaid that ignores the underlying sync blocking IO. Any configuration flag like this has to be explained to the users to make sure it is used in the right scenarios and then also be maintained and/or deprecated in the right ways. To me by reading through the conversation thread in the linked issue I couldn't really find derive that this writer loop bandaid will actually solve much in the wild. If the root cause should be fixed wouldn't it then be better to try to backport some of the asyncification from v7 that can be brought in as pure internal potentially to free up the code path from synchronous IO? |
|
I don't have objections to adopting this for The maintainers of this client understand that There is also a parallel to draw with the Khepri migration in RabbitMQ itself. RabbitMQ So I vote for merging this and focussing on |
lukebakken
force-pushed
the
rabbitmq-dotnet-client-1389
branch
from
1bbe3b8
to
aa847f0
Compare
| /// timeouts if a large number of blocking requests are going out simultaneously. Will become obsolete | ||
| /// once requests become asynchronous. Defaults to false. | ||
| /// </summary> | ||
| public bool EnableSynchronousWriteLoop { get; set; } = false; |
There was a problem hiding this comment.
Since the default value is false, maybe we could remove the init value
| public bool EnableSynchronousWriteLoop { get; set; } = false; | |
| public bool EnableSynchronousWriteLoop { get; set; } |
There was a problem hiding this comment.
Making the default more visible is a good idea. I'd not remove it.
There was a problem hiding this comment.
Overall, I think it's a kind of code style. I prefer to remove it while I would not argue for this.
Have a test on this, the default value would be removed when built in release mode, but would not when in debug mode
| if (_enableSynchronousWriteLoop) | ||
| { | ||
| TaskCreationOptions tco = TaskCreationOptions.LongRunning | TaskCreationOptions.DenyChildAttach; | ||
| _writerTask = Task.Factory.StartNew(SynchronousWriteLoop, CancellationToken.None, tco, TaskScheduler.Default); |
There was a problem hiding this comment.
There was a problem hiding this comment.
When just using the Task.Run, the thread would be borrowed from the thread pool, the long-running task on a thread-pool thread would affect the thread-pool thread scheduling.
Here's the LongRunning flag is specified, it would use a separate thread, so I think it's ok.
Fixes #1354
Supersedes #1389